Skip to content

fix: improve admin user search coverage#1466

Merged
ImLukeF merged 2 commits intomainfrom
fix/admin-user-search-recency
Apr 12, 2026
Merged

fix: improve admin user search coverage#1466
ImLukeF merged 2 commits intomainfrom
fix/admin-user-search-recency

Conversation

@ImLukeF
Copy link
Copy Markdown
Member

@ImLukeF ImLukeF commented Apr 1, 2026

Summary

Admin user search was only scanning a recent slice of users when a query was present. That meant an older existing account could be missing from the admin Users page even when the search text matched exactly.

This PR changes searched admin listings to scan the full ordered user set, while keeping the default non-search listing capped by limit. It also adds a regression test covering an older matching user outside the previous recent scan window.

Testing

Not run locally in this environment because bun is unavailable.

Copilot AI review requested due to automatic review settings April 1, 2026 23:11
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Apr 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Ready Ready Preview, Comment Apr 12, 2026 11:14am

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 1, 2026

Greptile Summary

This PR fixes a correctness bug where admin user searches missed older users that fell outside the previous 500–5 000 user bounded scan window, replacing take(n) with collect() in queryUsersForAdminList for non-empty queries.

  • The change affects both list and searchInternal (they share queryUsersForAdminList), but the two existing users.list tests that assert take(500) / take(2_000) were not updated — they will fail. The PR author notes tests were not run locally, so this was likely missed.

Confidence Score: 4/5

  • Safe to merge after fixing the two stale users.list test assertions that will fail under the new collect-based search path.
  • The logic change itself is correct and intentional. However, two tests in the unchanged users.list describe block still assert the old bounded-scan behaviour (take(500) / take(2_000)) and will fail because queryUsersForAdminList is shared. Since the author confirmed tests were not run, this slip is unsurprising but blocks a clean CI pass.
  • convex/users.test.ts — lines 873–919 in the users.list describe block need to be updated to match the new collect()-based search path.

Comments Outside Diff (2)

  1. convex/users.test.ts, line 873-919 (link)

    P1 Stale users.list search tests will fail after this change

    queryUsersForAdminList is shared by both list and searchInternal, so the switch from take(n) to collect() on the search path affects both handlers. Two tests in the users.list describe block were not updated and now assert the old bounded-scan behaviour:

    • "uses bounded scan for search instead of full collect" (line 873): asserts expect(take).toHaveBeenCalledWith(500) and expect(collect).not.toHaveBeenCalled() — both will fail because collect() is now called instead.
    • "clamps large limit and search scan size" (line 900): asserts expect(take).toHaveBeenCalledWith(2_000) — will fail for the same reason.

    Both tests should be updated to mirror the pattern established in the new searchInternal test: assert collect was called once and take was not.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: convex/users.test.ts
    Line: 873-919
    
    Comment:
    **Stale `users.list` search tests will fail after this change**
    
    `queryUsersForAdminList` is shared by both `list` and `searchInternal`, so the switch from `take(n)` to `collect()` on the search path affects both handlers. Two tests in the `users.list` describe block were not updated and now assert the old bounded-scan behaviour:
    
    - *"uses bounded scan for search instead of full collect"* (line 873): asserts `expect(take).toHaveBeenCalledWith(500)` and `expect(collect).not.toHaveBeenCalled()` — both will fail because `collect()` is now called instead.
    - *"clamps large limit and search scan size"* (line 900): asserts `expect(take).toHaveBeenCalledWith(2_000)` — will fail for the same reason.
    
    Both tests should be updated to mirror the pattern established in the new `searchInternal` test: assert `collect` was called once and `take` was not.
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. convex/users.ts, line 28-29 (link)

    P2 Dead constants after removing computeUserSearchScanLimit

    MAX_USER_SEARCH_SCAN and MIN_USER_SEARCH_SCAN are now unreferenced. The only consumer was the deleted computeUserSearchScanLimit function. They can be removed to keep the module clean.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: convex/users.ts
    Line: 28-29
    
    Comment:
    **Dead constants after removing `computeUserSearchScanLimit`**
    
    `MAX_USER_SEARCH_SCAN` and `MIN_USER_SEARCH_SCAN` are now unreferenced. The only consumer was the deleted `computeUserSearchScanLimit` function. They can be removed to keep the module clean.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: convex/users.test.ts
Line: 873-919

Comment:
**Stale `users.list` search tests will fail after this change**

`queryUsersForAdminList` is shared by both `list` and `searchInternal`, so the switch from `take(n)` to `collect()` on the search path affects both handlers. Two tests in the `users.list` describe block were not updated and now assert the old bounded-scan behaviour:

- *"uses bounded scan for search instead of full collect"* (line 873): asserts `expect(take).toHaveBeenCalledWith(500)` and `expect(collect).not.toHaveBeenCalled()` — both will fail because `collect()` is now called instead.
- *"clamps large limit and search scan size"* (line 900): asserts `expect(take).toHaveBeenCalledWith(2_000)` — will fail for the same reason.

Both tests should be updated to mirror the pattern established in the new `searchInternal` test: assert `collect` was called once and `take` was not.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: convex/users.ts
Line: 28-29

Comment:
**Dead constants after removing `computeUserSearchScanLimit`**

`MAX_USER_SEARCH_SCAN` and `MIN_USER_SEARCH_SCAN` are now unreferenced. The only consumer was the deleted `computeUserSearchScanLimit` function. They can be removed to keep the module clean.

```suggestion
const MAX_USER_LIST_LIMIT = 200;
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix admin user search coverage" | Re-trigger Greptile

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the admin user search behavior so that when a search query is provided it scans the full users dataset (rather than a bounded “recent users” window), while keeping the empty-query admin listing capped by limit. It also updates tests to prevent regressions where older matching users are missed.

Changes:

  • Switch admin search path from bounded take(...) scanning to full-table collect() scanning when a search query is present.
  • Keep empty-query listing behavior capped by limit.
  • Update/add regression coverage to ensure older matching users are included in results.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
convex/users.ts Changes admin list/search query behavior to collect() all users when searching.
convex/users.test.ts Updates regression tests to assert full-list search behavior and preserve empty-query cap behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

convex/users.ts Outdated
}

const scannedUsers = await orderedUsers.take(computeUserSearchScanLimit(args.limit));
const scannedUsers = await orderedUsers.collect();
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_USER_SEARCH_SCAN/MIN_USER_SEARCH_SCAN are now unused after removing computeUserSearchScanLimit. With noUnusedLocals: true in tsconfig, this will fail TypeScript compilation; remove these constants or reintroduce usage (e.g., as a cap for the search scan).

Suggested change
const scannedUsers = await orderedUsers.collect();
const scanLimit = clampInt(args.limit, MIN_USER_SEARCH_SCAN, MAX_USER_SEARCH_SCAN);
const scannedUsers = await orderedUsers.take(scanLimit);

Copilot uses AI. Check for mistakes.
Comment on lines 384 to 387
}

const scannedUsers = await orderedUsers.take(computeUserSearchScanLimit(args.limit));
const scannedUsers = await orderedUsers.collect();
const result = buildUserSearchResults(scannedUsers, normalizedSearch);
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling orderedUsers.collect() for every non-empty search performs an unbounded full-table scan of the users table. This will scale linearly with user count and may hit Convex execution/response limits as the dataset grows; consider adding a bounded/paginated scan strategy or a dedicated search/index-based approach instead of collecting all users at once.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 146e2bb3b1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

convex/users.ts Outdated
}

const scannedUsers = await orderedUsers.take(computeUserSearchScanLimit(args.limit));
const scannedUsers = await orderedUsers.collect();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore bounded scan for admin user search

Changing queryUsersForAdminList from a bounded take(...) to collect() makes every non-empty admin search read the entire users table, which can hit Convex query limits (document/bytes read caps, including the 32K-doc ceiling) as the user base grows. In that state, searchInternal/list with a search term will error instead of returning matches, so this is a functional regression plus a significant bandwidth/cost increase.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

ImLukeF commented Apr 12, 2026

Rebased onto current main, resolved the conflicts, and reran the local gates.

What’s on the branch now is the bounded-scan version plus exact handle / personal publisher lookup for older exact matches. It does not do an unbounded full-table collect on admin search.

Local verification after rebase:

  • bun x vitest run convex/users.test.ts src/__tests__/package-detail-route.test.tsx
  • bun run lint
  • bun run build

@ImLukeF ImLukeF merged commit cf137aa into main Apr 12, 2026
6 checks passed
BunsDev added a commit that referenced this pull request Apr 13, 2026
* build(deps-dev): bump vite in the npm_and_yarn group across 1 directory (#1561)

Bumps the npm_and_yarn group with 1 update in the / directory: [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite).


Updates `vite` from 8.0.1 to 8.0.5
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v8.0.5/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-version: 8.0.5
  dependency-type: direct:development
  dependency-group: npm_and_yarn
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: detect generated-source template injection in skill scans (#1597)

* fix: detect exposed resource identifiers in skill scans (#1598)

* fix: restore ci checks after lockfile drift

* refactor: address actionable review cleanup (#1601)

* fix: prevent starring soft-deleted skills and fix star count reconciliation (#1605)

* feat: Add support for Chinese Japanese and Korean(CJK) skills search (#1596)

Merged via squash.

Prepared head SHA: ab58f01
Co-authored-by: pq-dong <[email protected]>
Co-authored-by: momothemage <[email protected]>
Reviewed-by: @momothemage

* docs: document CLI config paths across platforms (#1252)

* docs: document CLI config paths across platforms

* docs: clarify legacy config fallback

---------

Co-authored-by: ImLukeF <[email protected]>

* fix: point plugin metadata help link to OpenClaw docs (#1399)

* fix: point plugin metadata help link to OpenClaw docs

* fix: open plugin metadata docs in a new tab

* fix(cli-auth): ensure fallback token renders before redirect on Windows/Chrome (#1486)

* fix(cli-auth): ensure fallback token renders before redirect on Windows/Chrome

React batches state updates, so setToken() and window.location.assign()
previously raced: the navigation could fire before React re-rendered the
fallback token UI. On Chrome/Windows this means a failed http:// redirect
(ERR_CONNECTION_REFUSED, HTTPS-first interference) would replace the page
with an error screen before the user ever saw the token.

Use flushSync() to render the token synchronously, then attempt
window.location.assign(). If the redirect fails the token and a "Retry
redirect to CLI" link are already painted on screen.

Fixes #1469

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* test: cover cli auth fallback redirect

---------

Co-authored-by: Claude Sonnet 4.6 <[email protected]>
Co-authored-by: ImLukeF <[email protected]>

* fix: reduce souls browse overfetch (#1637)

* fix: improve admin user search coverage (#1466)

* fix admin user search coverage

* fix admin user search without full table scan

* feat: include stats in package detail API response

Expose package detail stats through the shared API contract and the app client.

This lands the original package detail stats work and folds in the follow-up cleanup to keep the response shape sourced from the shared schema instead of a hand-maintained app-local type.

Co-authored-by: Saurabh Jain <[email protected]>

* test: cover package detail stats response

* fix: normalize misleading MIME types for text files

* feat: modernize clawhub app store

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Luke <[email protected]>
Co-authored-by: Momo <[email protected]>
Co-authored-by: pqdong <[email protected]>
Co-authored-by: Jholly <[email protected]>
Co-authored-by: loong <[email protected]>
Co-authored-by: Yaovi <[email protected]>
Co-authored-by: Claude Sonnet 4.6 <[email protected]>
Co-authored-by: Saurabh Jain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants